Repository navigation
crash_handler: truncate a panic message that does not fit the trace string instead of dropping it - #38950
crash_handler: truncate a panic message that does not fit the trace string instead of dropping it#38950robobun wants to merge 5 commits into
Conversation
…tring instead of dropping it encode_trace_string wrote the whole base64 zlib stream of the panic message in one append to the 1024-byte trace string buffer. When the header, frames and stream together exceeded the buffer, the append failed as a unit, the error was swallowed by TraceString's Display impl, and the report went out with a '0' reason and no payload, which bun.report cannot parse. The encoder now writes into the bounded buffer itself, so it knows how much room is left, and compresses the longest prefix of the message whose stream fits there (found by binary search, cut on a UTF-8 boundary), leaving space for the /view suffix when one follows. The crash_handler.panic() test hook takes an optional message so the test can drive this.
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for a maintainer. Reproduced on a debug build by giving the Test: CI: every lane that has run is green on the current head (177 of 179 jobs in build 98095, including Windows x64 and arm64 and the Linux ASAN lanes; the remaining entries flagged in the build are unrelated tests that passed on retry). The only job not accounted for is While probing this I found a separate problem, tracked separately: the crash handler's raw stderr writer ignores |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #38927, though the two touch the same files and should both land. #38927 changes how Rust This PR fixes the encoder, so every |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The binary-search truncation logic in write_panic_message and its interaction with utf8_prefix are non-trivial enough that a human look would still be worthwhile.
Checked: binary-search termination and the too_long == fits exit (only reachable when even the empty stream overflows the budget); utf8_prefix monotonicity so bisection stays well-ordered; that TRACE_STRING_MAX_LEN - writer.len() cannot underflow; that the OpenIssue path the test exercises has an empty suffix so the decoder's payload.slice(i+1) is exactly the base64; and that encode_len_from_size is const fn so B64_LEN compiles.
Extended reasoning...
Overview
This PR fixes a bug where a panic message that compressed to more than the room left in the 1024-byte trace-string buffer was silently dropped, leaving a 0 reason tag with no payload — which bun.report's decoder rejects, discarding the whole report. The fix changes encode_trace_string to take the concrete TraceStringBuf so it can measure remaining capacity, then binary-searches for the longest UTF-8-aligned prefix of the message whose zlib stream fits in the remaining space (less any /view suffix). Four files touched: src/crash_handler/lib.rs (encoder), src/runtime/api/crash_handler_jsc.rs and src/js/internal-for-testing.ts (test hook now accepts an optional message), and test/cli/run/run-crash-handler.test.ts (four new subprocess tests that decode the trace string end-to-end).
Security risks
None. This is diagnostic-output formatting on the crash path. No new inputs cross a trust boundary; the panic message was already being fed to zlib and base64. The only new unsafe block is the existing compress2 FFI call moved into a helper with the same bounds.
Level of scrutiny
Medium. The crash handler is not on any request-serving path, and the failure mode of a bug here is a malformed or over-truncated trace string while the process is already dying — no worse than the status quo, which drops the message entirely. That said, trace strings are how production crashes get reported, so a regression that broke them for common (short) messages would hurt debuggability. The two "encoded whole" tests guard that path, and the PR description confirms they pass on the unfixed encoder too.
Other factors
I traced the binary search's invariants: fits is only ever assigned a length whose (possibly backed-up) prefix has been proven to compress within budget, too_long is only assigned a length proven not to; len = fits + (too_long - fits)/2 keeps len in [fits, too_long), so the interval shrinks each iteration and too_long - fits cannot underflow. The break condition too_long - fits == 1 fires with a fresh compressed_len in hand. The too_long == fits early error is reachable only when even the empty zlib stream exceeds the budget (i.e. essentially zero room left), which is the same degenerate case the old code could not handle either. utf8_prefix is non-decreasing in len, so bisection stays well-ordered even though it may back up a few bytes; zlib non-monotonicity can only make the chosen prefix slightly sub-optimal, not incorrect. I also confirmed encode_trace_string's only caller is the Display impl (the two trace_str_buf sites go through it via write!), that the removed PartialEq/Eq derives are dead after the == became a match, and that the test's decoder correctly assumes no /view suffix because --debug-crash-handler-use-trace-string forces the OpenIssue action.
Deferring rather than approving because ~90 lines of new algorithmic Rust in the crash handler — a binary search over compressed sizes with a UTF-8 boundary adjustment — is past the threshold I'd auto-approve without a human sanity check, even though I found nothing wrong with it.
|
For whoever takes the human look the review above asks for, the parts worth reading closely are
|
|
Updated 12:15 PM PT - Aug 15th, 2026
❌ @robobun, your commit 1cf7ec0 has some failures in 🧪 To try this PR locally: bunx bun-pr 38950That installs a local version of the PR into your bun-38950 --bun |
|
Self-review raised one design question worth recording: why fit the message to the room left rather than just enlarge the buffer and cap the message at the producers (every in-tree producer is at or under 1024 bytes already). The PR body now has a bullet on that under Fix; the short version is that the header is not fixed size (the base URL comes from 1cf7ec0 also sizes the test's unfittable messages off |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds ~90 lines of new logic (a binary search over zlib compression attempts, plus an unsafe FFI call) to the crash handler — code that must not itself misbehave while the process is already dying — a human look at write_panic_message and utf8_prefix would still be worthwhile, as the author already noted.
What was reviewed:
- Binary-search termination in
write_panic_message:fits < too_longshrinks each iteration, thetoo_long == fitsguard catches the nothing-fits case, and the finaldest[..compressed_len]always holds the last successful compression. - Base64 budget math:
floor(3·budget/4)compressed bytes → unpadded base64 ≤ budget, andB64_LEN = encode_len_from_size(768) = 1024is large enough for the padded intermediate;reservedbytes are left for the suffix. utf8_prefix: Rust's&binds tighter than==, somessage[len] & 0xC0 == 0x80parses as intended; thelen < message.len()guard passes the whole message through untouched.TraceStringActionlosingPartialEq/Eq: no remaining==comparisons on it; all callers still go through theDisplayimpl, whose local 1024-byte buffer copies into an empty caller buffer of the same size.
Extended reasoning...
Overview
The PR fixes a bug where a panic message that compresses to more than the room left in the 1024-byte trace-string buffer was silently dropped (the all-or-nothing BoundedArray::append_slice failed and the error was discarded), leaving a bare 0 reason tag that bun.report's decoder rejects. The fix changes encode_trace_string to write into the concrete TraceStringBuf so it knows how much room remains, and adds write_panic_message, which binary-searches for the longest UTF-8-boundary prefix of the message whose zlib stream fits that room. It also introduces a TraceStringBuf type alias and TRACE_STRING_MAX_LEN constant so the two callers can't drift, extends the crash_handler.panic() test hook to accept a message, and adds four subprocess tests that decode the trace string end-to-end (walk header/VLQs/frames, inflate the payload with a fatal UTF-8 decoder).
Security risks
None identified. The input is a panic message that Bun itself produced; compress2 is called with correct pointer/length pairs (verified against the slice bounds); the Windows uLong narrowing is handled by treating a length that doesn't fit as "prefix doesn't fit". No user-controlled data reaches a new trust boundary.
Level of scrutiny
High. The crash handler runs after the process is already in a bad state, must not allocate, and must not itself crash — a bug here loses the very diagnostics it exists to preserve. The new code adds ~2.8 KB of stack locals (768 + 1024 in write_panic_message plus 1024 in the Display impl), an unsafe zlib FFI call in a loop, and a bisection whose termination depends on invariants I traced by hand rather than by test. That's beyond "mechanical", even though I believe it is correct.
Other factors
- The tests are well-constructed: entropy-derived messages guarantee incompressibility on every platform, the CJK case proves the cut lands on a char boundary via a fatal
TextDecoder, and the "encoded whole" cases pin the common path. The author verified the long-message cases fail on the unfixed encoder. - The comment-cop bot's flags were addressed (comments cut to one line each; all threads resolved).
- There is a known one-line merge overlap with #38927 (which deletes the
rust_panic_hookbuffer this PR retypes) that whoever lands second will need to resolve. - The
Displayimpl now double-buffers (encode into a local, then copy to the caller's buffer). Both are 1024 bytes and the caller's starts empty at every call site, so the copy always succeeds and behavior is preserved for non-panic reasons.
Problem
CrashReason::Panicwhose message compresses to more than the room left in the trace string (about 680 bytes on a typical Linux trace, less with more or longer frames) produces a trace string that ends right after the0reason tag: the frames are there, the message is gone, and nothing reports it. That string is what gets printed and whatreport()uploads. The pinned bun.report decoder (bun-tracestrings,parsePanicMessage) throws on the empty payload, so itsparse()returnsnulland bun.report rejects the whole report, frames included.encode_trace_string(src/crash_handler/lib.rs) compressed the message into a 2048-byte buffer, base64-encoded that into another 2048-byte buffer, and appended the result with a singlewrite_allto the 1024-bytetrace_str_bufits callers render into.BoundedArray::append_sliceis all-or-nothing, so once header + frames + base64 exceeded 1024 bytes the append failed as a unit, andTraceString'sDisplayimpl discarded the error (let _ = encode_trace_string(..)). The two 2048-byte intermediates only hid the real limit.crash_handler.panic()test hook: a 2048-character incompressible ASCII message and a 1024-character CJK message both produced a 117-byte trace string with an empty payload; 900, 1000 and 1024 characters of base64-alphabet text (all within the 1024-byte cap the Rust panic hook applies) did not fit either. An 11-character message produced a 143-byte trace string that decoded normally.Fix
encode_trace_stringwrites into theTraceStringBuf(BoundedArray<u8, TRACE_STRING_MAX_LEN>) itself instead of an arbitrary writer, so when it reaches the message it knows how much room is left.write_panic_messageturns that room (minus the/viewsuffix when one follows) into a maximum compressed size and compresses the longest prefix of the message whose zlib stream fits: the whole message is tried first, and only if that fails does it bisect the cut. The cut is backed up to a UTF-8 character boundary (utf8_prefix); a message that fits is passed through byte for byte.BUN_CRASH_REPORT_URLverbatim; each of the up to 20 frames that falls outside bun's own image carries that module's file name on Windows (StackLine::from_address), so a crash with its frames inside a native addon with a 30-character file name can put several hundred bytes of names ahead of the message; crash_handler: put the executable's debug id in the trace string #38838 adds more header fields. With a few hundred bytes of header even an ordinary 1 KBunwrap()message no longer fits today, and any fixed split between buffer size and message cap can be pushed past the same way by a longer base URL or longer module names, which is how the 2048-byte intermediates came to be wrong. Computing the budget from the bytes actually left is the only shape under which the encoder cannot emit an undecodable string; it also makesTRACE_STRING_MAX_LENa plain knob (raising it to keep more of long messages is a one-line change that is safe either way), and it coverspanic_implandBun__crashHandler, whose messages are not capped anywhere.compress2calls, each the same one-shot call (and the same internaldeflateInitallocation) the current code makes once; zlib stops as soon as the output buffer (at most 768 bytes) is full, so no attempt scales with the message. Scratch space is 768 + 1024 bytes plus a 1024-byte local in theDisplayimpl, down from 2048 + 2048.TraceString'sDisplayimpl encodes into a localTraceStringBufand forwards it, so the five existingTraceStringsites (two panic paths, threeZigErrorstderr paths) are unchanged, and an encoding error still leaves whatever was encoded to be printed, as before.TraceStringActionloses itsPartialEqderive, which the removed==was the only user of.trace_str_bufdeclarations becomeTraceStringBufso they cannot drift from the encoder's limit. The one inrust_panic_hookis deleted by crash_handler: report Rust panics through crash_handler() and test the panic hook #38927, which routes Rust panics throughcrash_handler(), so merging the two PRs needs that one line resolved. crash_handler: report Rust panics through crash_handler() and test the panic hook #38927 also caps the hook's message at 1024 bytes; as measured above that alone does not avoid this bug, and with this change any length yields a decodable report.crash_handler.panic()inbun:internal-for-testingtakes an optional message (src/runtime/api/crash_handler_jsc.rs). Without one it panics with the same message as before, so the existing assertions on it are unaffected.trace string panic message: decodes the trace string by walking the format (header, feature VLQs, frames,0tag) and inflating the payload with a fatal UTF-8 decoder. A short and an empty message decode whole. Two messages built fromTRACE_STRING_MAX_LENbytes of hash output (as hex, and as one 3-byte CJK character per byte) decode to a non-empty strict prefix of the message, with the trace string within 64 bytes of the limit, so the cut is the longest one that fits rather than an arbitrary shorter one; the CJK case also proves the cut lands on a character boundary. Since the payload can hold at most 3/4 ofTRACE_STRING_MAX_LENcompressed bytes and that much entropy cannot compress below it, these messages cannot fit on any platform or header size, and a future change to the constant keeps them unfittable.panic reason has no payload: the message was dropped; with the released bun all four fail because the hook ignores its argument; withbun bd testall four pass (the hex message is cut to 1213 characters, the CJK one to 413, both trace strings exactly 1024 bytes). The whole file passes (21 pass, 9 platform skips), as does test/cli/run/crash-report-command-char.test.ts; the Windows x64 and arm64 CI lanes ran the new cases as well.cargo check -p bun_crash_handlerfor x86_64-pc-windows-msvc, aarch64-pc-windows-msvc, aarch64-apple-darwin and x86_64-unknown-linux-gnu (uLongis 32-bit on Windows;compress_messagetreats a length that does not fit it as a prefix that does not fit), andcargo clippyon the crate is clean.Background
https://bun.report/<version>/<payload>URL the crash handler prints and optionally uploads. The payload is positional: platform and command characters, a format version character, 7 characters of git sha, then VLQ-encoded fields (feature bits, one entry per stack frame, a 0 terminator), then a reason character and its payload. A frame outside bun's own image is encoded as the module name plus an offset, which is the variable-size part of the header. For a panic the reason is0and the payload is the zlib stream of the message in base64 with the padding stripped; bun.report's decoder inflates everything after the tag. The string is assembled in a stack buffer in the crash handler, which is where the 1024-byte limit comes from.BoundedArray<u8, N>: a fixed-capacity array plus a length, used so the crash path does not allocate. Itswrite_allappends the whole slice or nothing.ceil(4n / 3)bytes forninput bytes, which is wherebudget * 3 / 4, the largestnwhose encoding fits inbudgetbytes, comes from.compress2is zlib's one-shot compressor. When the output buffer is too small it returnsZ_BUF_ERRORas soon as the buffer is full instead of compressing the rest of the input, which is what keeps the retries cheap.